Skip to content

Add Elixir/Erlang version feature and respective feature test#138

Merged
matiaswilner merged 4 commits intomasterfrom
show_version
Sep 20, 2021
Merged

Add Elixir/Erlang version feature and respective feature test#138
matiaswilner merged 4 commits intomasterfrom
show_version

Conversation

@matiaswilner
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Contributor

@eldano eldano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this approach as it looks similar to iex.
I also thought about displaying this info in a more global place like near the heading of the page. Just wondering if you considered other cases too

Comment thread lib/elixir_console_web/live/console_live/history_component.html.leex Outdated
Comment thread lib/elixir_console_web/live/console_live/history_component.html.leex Outdated
end)
end

feature "visitor gets Elixir and Erlang version", %{session: session} do
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this is ok, I don't know if we want to add too many feature tests as these are slow. In particular, I'm not very concerned about introducing a regression here but this is questionable. Thoughts? cc/ @grzuy

Copy link
Copy Markdown
Contributor

@grzuy grzuy Sep 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, generally agree.

I think in this particular case, this a functionality that can be left out of the feature test coverage without losing a lot given the implementation is quite simple and straightforward, meaning there's low probability of introducing a bug, and if still buggy, it would be something can be easily detected visually by just rendering the page.

@matiaswilner Thanks anyway for trying to have coverage for this, better to err on the side of coverage than not.

matiaswilner added 2 commits September 17, 2021 15:46
-Delete feature test
-Add class to style version-info div
-Improve code-styling issue
@command_output css("#commandOutput")
@suggestions_list css("#suggestions-list")
@documentation_output css("#documentation-output")
@version_info css("#version-info")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now this module attribute is not needed anymore. Thanks for removing that test!

@matiaswilner matiaswilner merged commit 7f9ea4a into master Sep 20, 2021
@matiaswilner matiaswilner deleted the show_version branch September 20, 2021 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants